presenting the Oscar winning welcome page #894

cezaraugusto · 2018-11-12T22:41:45Z

closes brave/brave-browser#1847

npm run test-unit should cover edge cases of welcome page
brave://welcome for manual testing

all props to @rossmoody I just did the code

bbondy · 2018-12-02T15:36:17Z

@cezaraugusto ping on this since this is 22 days old

petemill

This is very cool!

Left some comments, mainly nits and some tiny cleanup.

One thing would be great to do is move the images from brave-core/components/img to as local to where they are used as possible, continuing with out as much as possible self-containing component architecture. e.g. if the asset is imported in brave_welcome_ui/components/screens, lets put the images there. If it's actually needed by a brave-ui component and it never changes (e.g. <ShieldsImage />), then we can leave the assets in brave-ui and we don't need the additional complication of passing the url down as a prop. The other thing is that we don't need to specify the generated image names and paths in c++ - webpack will put the files in the right place, and the build will pick them up and make sure they are served at runtime.
If you do change these, please use import myUrl from 'my.svg' syntax instead of require. It's good for us to consistently use es-module style instead of nodejs style for webpack which only supports tree shaking and code splitting using es module syntax.
If you don't, please at least remove the image definitions from the GRD and c++.

Have the SVG graphics been optimized with svgo(mg)? It's probably yes because these are complicated images so that may be why they are still big, so just checking... If not, please do.

Second, a UX point: when experiencing this page for the first time, I wonder if the transitions feel pretty slow. The brave logo hovering timing seems perfect. The initial fade-in seems maybe a little slow and the slide animation feels very slow. We've got the slide animation at around 2.5s. Various research I've read in the past suggests micro-transitions should be around 250ms or less and more general page transitions like this could be double that and at least < 1s. Should easing be ease-in-out (or some variant) since content is both exiting and entering. I think we have that set to ease-out.
If you and @rossmoody don't feel that this needs to be changed then ok. If you want to adjust this in brave-ui before pushing this PR, then go for it. If you want to do it later or want someone else to do it, then we can merge this as long as @rossmoody is happy with the timings as they are potentially going in to a Release.

petemill · 2018-12-05T03:19:15Z

browser/ui/webui/brave_webui_source.cc

-      { "59ec9e5cdea1df1630f8c6e2b7a27ede.png", IDR_BRAVE_WELCOME_SLIDE_4_IMAGE },
-      { "c2217e15737be3c82f5fef818d3fd26c.png", IDR_BRAVE_WELCOME_SLIDE_5_IMAGE },
-      { "e9d936c617aad55fe1bc7a001f2defb4.svg", IDR_BRAVE_WELCOME_BACKGROUND_IMAGE }
+      { "9e31ed86a2f12b6d4a22041f62ed412e.svg", IDR_BRAVE_WELCOME_SLIDE_1_IMAGE },


These images don't need to be specified here, they will be automatically included if generated via webpack build

petemill · 2018-12-05T03:23:06Z

components/brave_welcome_ui/components/app.tsx

-  get actions () {
-    return this.props.actions
+  get totalScreensSize () {
+    return 6


nit: this can be a const variable in the module, instead of a function, since it does not change

petemill · 2018-12-05T04:02:53Z

components/brave_welcome_ui/components/app.tsx

-      default:
-        return <BraveScreen onGoToFirstSlide={this.onGoToFirstSlide} />
-    }
+  get backgroundPosition () {


this does not seem to be called from anywhere - can be removed

petemill · 2018-12-05T04:06:42Z

components/brave_welcome_ui/components/app.tsx


 // Utils
 import * as welcomeActions from '../actions/welcome_actions'

 // Assets
-const background = require('../../img/welcome/welcomebg.svg')
+const background = require('../../img/welcome/welcome_bg.svg')


nit: the webpack loaders will return a URL string for asset file imports, so a more readable name IMO would be backgroundUrl in these cases

petemill · 2018-12-05T04:29:11Z

components/brave_welcome_ui/components/screens/footerBox.tsx

+
+// Shared components
+import { ArrowRightIcon } from 'brave-ui/components/icons'
+import { Button } from 'brave-ui/components'


please use import { Button } from 'brave-ui' for consistency in order to preserve tree shaking. At least the important thing is to be consistent within a brave-core webui, so if you want to leave it as this for all of them, then that should be technically ok.

components/brave_welcome_ui/components/screens/searchBox.tsx

petemill · 2018-12-05T04:54:49Z

components/brave_welcome_ui/components/screens/themeBox.tsx

+import { Content, Title, ThemeImage, Paragraph } from 'brave-ui/features/welcome/'
+
+// Shared components
+import { Button } from 'brave-ui/components'


remove /components to be consistent

petemill · 2018-12-05T04:56:24Z

components/img/welcome/welcome_import.svg

@@ -0,0 +1,164 @@
+<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 325 200">


Has this been optimized with svgo(mg)? It's probably yes because these are complicated images so that may be why they are still big, so just checking...

can speak to this one cause i did it. these were run through svgo but they are pretty serious svgs. quite a few elements with linear gradients in each.

petemill · 2018-12-05T04:57:53Z

components/resources/brave_components_resources.grd

-      <include name="IDR_BRAVE_WELCOME_SLIDE_4_IMAGE" file="../img/welcome/rewards.png" type="BINDATA" />
-      <include name="IDR_BRAVE_WELCOME_SLIDE_5_IMAGE" file="../img/welcome/shields.png" type="BINDATA" />
-      <include name="IDR_BRAVE_WELCOME_BACKGROUND_IMAGE" file="../img/welcome/welcomebg.svg" type="BINDATA" />
+      <include name="IDR_BRAVE_WELCOME_SLIDE_1_IMAGE" file="../img/welcome/lion_logo.svg" type="BINDATA" />


These don't need to be specified if they are imported via webpack, and from what I can tell, they are.

petemill · 2018-12-05T04:58:50Z

components/resources/brave_components_strings.grd

      <message name="IDS_BRAVE_WELCOME_PAGE_MAIN_BUTTON" desc="Button that moves to the next welcome screen">Let's go</message>
      <message name="IDS_BRAVE_WELCOME_PAGE_REWARDS_TITLE" desc="Welcome message title for Brave Rewards">Enable Brave Rewards</message>
-      <message name="IDS_BRAVE_WELCOME_PAGE_REWARDS_DESC" desc="Explainer text about Brave Rewards">Brave Rewards is a new way of thinking about how the web works. Get rewarded for viewing privacy-respecting ads, and support your favorite content creators while you’re at it.</message>
+      <message name="IDS_BRAVE_WELCOME_PAGE_REWARDS_DESC" desc="Explainer text about Brave Rewards">Your attention is valuable. Earn by viewing privacy-respecting ads while you browse, and pay it forward to support content creators you love.</message>


Thanks for putting in good descs 👍

components/brave_welcome_ui/BUILD.gn

rossmoody · 2018-12-05T05:55:13Z

@cezaraugusto Per animation notes: The initial hope was to give the user a few seconds to adjust (since technically this would be the first time opening Brave) and also the panels are pretty big but I think I'd prefer to be a little fast than a little slow and we can always fine tune after this. Could you adjust the transition timing for the panels and the initial reveal to 400ms? We can keep the ease designations the same.

petemill

LGTM, thanks for making the changes 🎆

presenting the Oscar winning welcome page

petemill · 2018-12-12T17:59:34Z

0.60.x 4bf00c4

petemill · 2018-12-12T18:06:16Z

@rebron @kjozwiak @srirambv uplift request for 59 (or 58)

cezaraugusto · 2018-12-12T18:57:43Z

@petemill approved for 59 per https://bravesoftware.slack.com/archives/C7VLGSR55/p1544639764749500

kjozwiak · 2018-12-12T19:05:05Z

@petemill uplift request to 0.59.x beta approved 👍 Please add/remove all needed labels and ensure that the associated issue is moved to the correct milestone.

presenting the Oscar winning welcome page

cezaraugusto · 2018-12-19T23:14:21Z

0.59.x cb0fd9b

cezaraugusto self-assigned this Nov 12, 2018

cezaraugusto requested review from petemill, bsclifton and rossmoody November 12, 2018 22:41

cezaraugusto force-pushed the welcome-v2 branch from 7629f9a to 760703b Compare November 12, 2018 23:06

cezaraugusto force-pushed the welcome-v2 branch 2 times, most recently from c0c68b5 to 25bfe83 Compare December 4, 2018 19:14

petemill force-pushed the welcome-v2 branch from 25bfe83 to e8db428 Compare December 5, 2018 05:00

petemill requested changes Dec 5, 2018

View reviewed changes

petemill reviewed Dec 5, 2018

View reviewed changes

components/brave_welcome_ui/BUILD.gn Show resolved Hide resolved

cezaraugusto added 4 commits December 11, 2018 19:27

add new resouces (images/strings) to welcome page

570dd44

welcome page v2

24aa3db

remove legacy files

954e144

update tests for welcome page

136c226

cezaraugusto force-pushed the welcome-v2 branch from e8db428 to 136c226 Compare December 11, 2018 22:02

petemill approved these changes Dec 12, 2018

View reviewed changes

petemill merged commit 208606f into master Dec 12, 2018

petemill deleted the welcome-v2 branch December 12, 2018 17:58

petemill added the merged/0.61.x-Nightly label Dec 12, 2018

petemill added a commit that referenced this pull request Dec 12, 2018

Merge pull request #894 from brave/welcome-v2

4bf00c4

presenting the Oscar winning welcome page

petemill added merged/0.60.x-Dev and removed merged/0.61.x-Nightly labels Dec 12, 2018

kjozwiak added the uplift-approved/0.59.x-Beta label Dec 12, 2018

kjozwiak removed the uplift-request/0.59.x-Beta label Dec 12, 2018

cezaraugusto pushed a commit that referenced this pull request Dec 19, 2018

Merge pull request #894 from brave/welcome-v2

cb0fd9b

presenting the Oscar winning welcome page

cezaraugusto added merged/0.59.x-Beta and removed uplift-approved/0.59.x-Beta labels Dec 19, 2018

bbondy added this to the 0.59.x - Beta milestone Jan 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

presenting the Oscar winning welcome page #894

presenting the Oscar winning welcome page #894

cezaraugusto commented Nov 12, 2018

bbondy commented Dec 2, 2018

petemill left a comment

petemill Dec 5, 2018

petemill Dec 5, 2018

petemill Dec 5, 2018

petemill Dec 5, 2018

petemill Dec 5, 2018

petemill Dec 5, 2018

petemill Dec 5, 2018

rossmoody Dec 5, 2018

petemill Dec 5, 2018

petemill Dec 5, 2018

rossmoody commented Dec 5, 2018

petemill left a comment

petemill commented Dec 12, 2018

petemill commented Dec 12, 2018

cezaraugusto commented Dec 12, 2018

kjozwiak commented Dec 12, 2018

cezaraugusto commented Dec 19, 2018

		@@ -0,0 +1,164 @@
		<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 325 200">

presenting the Oscar winning welcome page #894

presenting the Oscar winning welcome page #894

Conversation

cezaraugusto commented Nov 12, 2018

bbondy commented Dec 2, 2018

petemill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossmoody commented Dec 5, 2018

petemill left a comment

Choose a reason for hiding this comment

petemill commented Dec 12, 2018

petemill commented Dec 12, 2018

cezaraugusto commented Dec 12, 2018

kjozwiak commented Dec 12, 2018

cezaraugusto commented Dec 19, 2018